Skip to content

misra.py: R14.2: Verify for loop counter modification#2409

Merged
danmar merged 14 commits into
cppcheck-opensource:masterfrom
jubnzv:misra-14-2-fix
Dec 15, 2019
Merged

misra.py: R14.2: Verify for loop counter modification#2409
danmar merged 14 commits into
cppcheck-opensource:masterfrom
jubnzv:misra-14-2-fix

Conversation

@jubnzv
Copy link
Copy Markdown
Contributor

@jubnzv jubnzv commented Nov 29, 2019

Add additional check to detect modification of loop counter in loop body. Related issue: https://trac.cppcheck.net/ticket/9490

Add small fix that treat all assignment operators defined in N1750 6.5.16 as has side affects. This will affects rules 13.1, 13.3, 13.5 and allow to catch some false negatives.

Add additional check to detect modification of loop counter in loop
body. Related issue: https://trac.cppcheck.net/ticket/9490

Add small fix that treat all assignment operators defined in N1750
6.5.16 as has side affects. This will affects rules 13.1, 13.3, 13.5
and allow to catch some false negatives.
Comment thread addons/test/misra/misra-test.c Outdated
Comment thread addons/misra.py Outdated
Comment thread addons/misra.py Outdated
@versat
Copy link
Copy Markdown
Collaborator

versat commented Nov 30, 2019

I have merged the PR with the 14.2 tests. Can you merge Head into this PR please.

Comment thread addons/misra.py Outdated
Copy link
Copy Markdown
Collaborator

@versat versat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok to me now 👍

Comment thread addons/misra.py
Comment thread addons/misra.py Outdated
@jubnzv
Copy link
Copy Markdown
Contributor Author

jubnzv commented Dec 2, 2019

@danmar does Cppcheck supports multiple variables declaration in for scope? I found that j variable was not generated in XML dump at these lines: https://github.com/danmar/cppcheck/pull/2409/files#diff-dee0960ff8ec5a1387684d43113107a4R494-R497.

N1750 allows this declaration at 6.8.5.3:

  1. Thus, clause-1 specifies initialization for the loop, possibly declaring one or more variables for use in the loop;

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Dec 4, 2019

@danmar does Cppcheck supports multiple variables declaration in for scope? I found that j variable was not generated in XML dump at these lines:

Yes of course we should support that. Something is missing in cppcheck for that. Strange that we have not noticed before! Awesome find! 👍

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Dec 4, 2019

I created the ticket https://trac.cppcheck.net/ticket/9516

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Dec 6, 2019

I am still not sure about the getLoopCounterVariables. As far as I see, your current code says that there are no loop counter variables here:

for (x = 0; x < 10; x++)

Section "8.14" in the misra pdf contains a definition for a loop counter variable;

  • it has scalar type
  • Its value varies monotonically on each iteration of a given instance of a loop; and
  • it is involved in a decision to exit the loop

So for instance, there is no loop counter variable here:

for (x=0; y < rand(); z++) {}

@jubnzv
Copy link
Copy Markdown
Contributor Author

jubnzv commented Dec 10, 2019

@danmar that's right, this PR adds an additional check for variables that defined in the first clause of for loop. This is required to avoid some false negatives in current implementation.

We can add more checks for loop counters from outer scopes and loop control flags defined in MISRA document as well. But it will be a bit more complicated due to lack of concrete examples and some unclear definitions in MISRA. I suppose it should be implemented incrementally in other PRs.

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Dec 11, 2019

if I have to choose false negatives or false positives I'd choose false negatives. We can definitely try to solve this incrementally... if there are no false positives and we reduce false negatives.

Do you believe that there will not be false positives caused by this PR?

To avoid false positives it seems to me that the getForLoopCounterVariables should only return a variable that meets all the specified requirements. To check it is involved in a decision to exit the loop, I guess it's enough to check if it's used anywhere in second for loop expression.

@jubnzv
Copy link
Copy Markdown
Contributor Author

jubnzv commented Dec 11, 2019

I think I got what you mean. Are you about the case when we declare variable in first clause of for loop and it's value doesn't involved in decision to exit loop? This variable doesn't satisfy loop counter definition for MISRA document and it's modification is allowed. Something like this:

int y = 0; // loop counter
for (int i = 0; y < 10; y++) {
  i++; // 14.2. false positive?
}

Am I right?

But in this case we should generate violation of rule 14.2 in for loop, according following restrictions:

First clause which
• Shall be empty, or
• Shall assign a value to the loop counter, or
• Shall define and initialize the loop counter (C99).

So we want:

int y = 0;
for (int i = 0; y < 10; y++) { // 14.2. violation in first clause
  i++; // no warning
}

And in the following case:

int y = 0;
for (int i = 0; y < 10; y++) {
  if (++i > 5) { // 14.2
    break;
  }
}

i variable is completely satisfies loop counter definition, but there is modification in loop body. So, we need to generate violation in if line.

Okay, I'll check it a little later.

@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Dec 11, 2019

yes I agree with that.

@jubnzv
Copy link
Copy Markdown
Contributor Author

jubnzv commented Dec 15, 2019

@danmar I've corrected it according to your review.

@danmar danmar merged commit c46e44e into cppcheck-opensource:master Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants